Skip to content

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 27, 2023

@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2023
We are operating on u8 (aka. byte) values here. ASCII only allows values from 0-127.

This would also be consistent with the naming of `OsStr::from_bytes()`.
@Turbo87 Turbo87 force-pushed the rename-parse-ascii branch from 470714f to 7783695 Compare October 27, 2023 10:23
@cuviper
Copy link
Member

cuviper commented Oct 27, 2023

We are operating on u8 (aka. byte) values here. ASCII only allows values from 0-127.

Are any non-ASCII bytes actually accepted Ok though?

This would also be consistent with the naming of OsStr::from_bytes().

That's an infallible conversion, truly accepting any bytes.

Anyway, I think this changes posture enough that it needs to be an API decision, especially given that there's a proposed generalization in play at rust-lang/libs-team#287.

@rustbot label -T-libs +T-libs-api
r? libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2023
@rustbot rustbot assigned joshtriplett and unassigned cuviper Oct 27, 2023
@Turbo87
Copy link
Member Author

Turbo87 commented Oct 27, 2023

Are any non-ASCII bytes actually accepted Ok though?

fair point, I guess... 🤔

@jmillikin
Copy link
Contributor

jmillikin commented Oct 28, 2023

It seems confusing for IpAddr::from_bytes(&[58, 58, 48, 49]) and IpAddr::from([58, 58, 48, 49]) to have different behavior .

@scottmcm
Copy link
Member

When I see "from bytes" I think https://doc.rust-lang.org/std/primitive.u32.html#method.from_ne_bytes, not anything related to parsing. So I think it needs more in the name to clarify the intent of the bytes.

@Turbo87 Turbo87 closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants